Gerard/4372 intersection issue found using create bar #4527

ggartside · 2022-02-03T19:24:14Z

Pull request overview

Fixes Intersection issue found when using create bar #4372

Fixes an issue revealed by surface matching when small surfaces created in intersection were reduced to 0 area surfaces and discarded. The tolerance used in removeSpikesEx should have been the same value as the amount used to shrink and expand surfaces to remove spikes

PR includes the fix (one line change) a test model and a unit test in Model_Gtest to verify the surfaces are paired correctly

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

…rrect

… tolerance used in removeSpikesEx collapsed narrow aspect ratio surfaces to 0 area surfaces

ggartside · 2022-02-03T19:42:53Z

The issue was a fault in removeSpikesEx. In this method a polygon is expanded by amount and then shrunk by amount to remove small spikes, the resulting vertices were compared to the original vertices and adjusted accordingly so that the original coordinates could be returned wherever possible. The tolerance used to compare vertices was too large and resulted in the polygon being reduced to a 0 area polygon and rejected. By changing the tolerance to the amount expanded and shrunk the issue was fixed.

Polygons are rejected based on area tolerance in intersection - they have to be greater than tol * tol which is 100mm2, surface matching uses a linear tolerance of 0.01, 10mm. If a polygon is rejected because it is less than 100mm2 it is possible that vertices in that polygon could be > 10mm apart and hence surface matching could fail under certain circumstances.

Currently, if two identical surfaces are intersected their vertices are not changed by the intersection method, which makes perfect sense. However if two almost identical surfaces are intersected and the resulting difference between them is too small to be considered then the original surface vertices are not changed either even though they are slightly different.

These two surfaces are therefore considered to be identical after intersection is performed but may not be considered identical as far as surface matching is concerned (because they are not identical)

To eliminate this error intersection should always return two identical surfaces that will be determined to be equal when passed to circularEqual

This reverts commit 60268fe.

…n also do a circularEqual check to make sure these two surfaces will macth correctly

ggartside · 2022-02-08T22:08:05Z

I resolved the two test fails. One of them was caused by adding the circularEqual test in Surface intersect. The circulaEqual is removing inline points and so the count of vertices dropped from 6 to 4. This may be an unwanted artifact so to be prudent it was removed

…g-create-bar

jmarrec

Some minor changes, otherwise everything looks good!

src/model/test/Model_GTest.cpp

src/model/test/Space_GTest.cpp

src/utilities/geometry/Intersection.cpp

…nce th eorder the polygons are generated in such that newPolygons[0][ and newPolygons[1] are swapped

Co-authored-by: Julien Marrec <[email protected]>

Yeah my bad :( Co-authored-by: Julien Marrec <[email protected]>

I presume nameString() calls name().value()? I will use nameString() in future - less typing Co-authored-by: Julien Marrec <[email protected]>

Co-authored-by: Julien Marrec <[email protected]>

https://github.com/NREL/OpenStudio into gerard/4372_Intersection-issue-found-using-create-bar

…rent surfaces all work

Co-authored-by: Julien Marrec <[email protected]>

ci-commercialbuildings · 2022-02-09T22:25:07Z

CI Results for 3867acb:

tijcolem

@jmarrec Thanks for reviewing. Just running the windows CI again and if that passed I'll merge.

tijcolem and others added 5 commits June 24, 2021 12:46

comment out rc1 and prep for 3.2.1 release

bdbdbc9

Merge branch '3.3.0-rc3'

b79cf67

Unit test to validate that areas before and after intersection are co…

d5110b7

…rrect

Fixes 4372: Intersection issue found when using create bar. INcorrect…

05edfe9

… tolerance used in removeSpikesEx collapsed narrow aspect ratio surfaces to 0 area surfaces

Added the test model to the Cmake file

554acf8

ggartside requested review from DavidGoldwasser, jmarrec and tijcolem February 3, 2022 19:24

ggartside added 4 commits February 3, 2022 14:04

clang format changes (the headers are nothing to do with me)

60268fe

Revert "clang format changes (the headers are nothing to do with me)"

0f6bb49

This reverts commit 60268fe.

Clang fixes

4baca3d

If the two surfaces intersect with no additional polygons created the…

2d89b73

…n also do a circularEqual check to make sure these two surfaces will macth correctly

tijcolem added severity - Major Bug component - Utilities Geometry Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Feb 4, 2022

Resolves the two test fail issues

49e34b2

Merge branch 'develop' into gerard/4372_Intersection-issue-found-usin…

1547e88

…g-create-bar

jmarrec requested changes Feb 9, 2022

View reviewed changes

ggartside and others added 10 commits February 9, 2022 10:24

removeSpikesEx has changed the order the vertices are returned and he…

4b0d28f

…nce th eorder the polygons are generated in such that newPolygons[0][ and newPolygons[1] are swapped

Update src/model/test/Model_GTest.cpp

4c3ffa3

Co-authored-by: Julien Marrec <[email protected]>

Update src/model/test/Model_GTest.cpp

e898e63

Yeah my bad :( Co-authored-by: Julien Marrec <[email protected]>

Update src/model/test/Model_GTest.cpp

052baf9

I presume nameString() calls name().value()? I will use nameString() in future - less typing Co-authored-by: Julien Marrec <[email protected]>

Update src/model/test/Model_GTest.cpp

54b3379

Co-authored-by: Julien Marrec <[email protected]>

Responding to PR comments

b5b7cde

Merge branch 'gerard/4372_Intersection-issue-found-using-create-bar' of

4bfc34f

https://github.com/NREL/OpenStudio into gerard/4372_Intersection-issue-found-using-create-bar

Resolving test failures

91433dd

Fixes remaining tests so subs urface overlaps and extending beyond pa…

f3d19c1

…rent surfaces all work

Uggh clang

1acdc21

ggartside and others added 2 commits February 9, 2022 16:09

Update src/model/test/Model_GTest.cpp

c5841f6

Co-authored-by: Julien Marrec <[email protected]>

Update src/model/test/Model_GTest.cpp

3867acb

Co-authored-by: Julien Marrec <[email protected]>

tijcolem approved these changes Feb 14, 2022

View reviewed changes

tijcolem merged commit e7ec8d4 into develop Feb 18, 2022

tijcolem deleted the gerard/4372_Intersection-issue-found-using-create-bar branch February 18, 2022 15:06

tijcolem mentioned this pull request Aug 26, 2022

Intersection issue impacting create bar measure NREL/openstudio-extension-gem#128

Open

brgix mentioned this pull request Apr 7, 2024

Autogenerate skylight wells (redux) rd2/osut#17

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gerard/4372 intersection issue found using create bar #4527

Gerard/4372 intersection issue found using create bar #4527

ggartside commented Feb 3, 2022 •

edited

Loading

ggartside commented Feb 3, 2022 •

edited

Loading

ggartside commented Feb 8, 2022

jmarrec left a comment

ci-commercialbuildings commented Feb 9, 2022 •

edited

Loading

tijcolem left a comment

Gerard/4372 intersection issue found using create bar #4527

Gerard/4372 intersection issue found using create bar #4527

Conversation

ggartside commented Feb 3, 2022 • edited Loading

Pull request overview

Pull Request Author

Review Checklist

ggartside commented Feb 3, 2022 • edited Loading

ggartside commented Feb 8, 2022

jmarrec left a comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Feb 9, 2022 • edited Loading

tijcolem left a comment

Choose a reason for hiding this comment

ggartside commented Feb 3, 2022 •

edited

Loading

ggartside commented Feb 3, 2022 •

edited

Loading

ci-commercialbuildings commented Feb 9, 2022 •

edited

Loading